Review: PR #472 — Fix port-reservation race in libtest::Server::start()

Overview

This PR fixes the flaky-CI root cause described in #471: get_free_port() was releasing its reservation socket immediately, and the release_port() call in Server::start() was consequently a no-op (the socket had already been closed). The fix has two parts:

• libtest/port.cc: get_free_port() no longer closes its reservation socket — it stays bound (not listening) until the caller explicitly calls release_port().
• libtest/server.cc: Server::start() moved the release_port(_port) call from right before _app.run() to right after the ping loop confirms the child is listening or has definitively failed.

The core technique is sound: gearmand.cc:521 sets SO_REUSEADDR on its real listening socket, so the "bind succeeds while reservation is held, since neither side calls listen()" claim in the PR description checks out on Linux.

Code quality and style

• Small, surgical diff (11/-3 lines across 2 files) that matches the project's existing style.
• The new comments in both files clearly explain why the release was moved/removed, which is exactly the kind of non-obvious rationale worth documenting.

Correctness issue (real regression)

Server::start() (libtest/server.cc:195-364) has several throw paths between where the reservation is taken (before this function runs, via get_free_port()) and the new release_port(_port) call at line 311:

• args(_app) == false → throws at line 228 (pre-existing, was already before the old release call too — no regression here)
• _app.run() failure → throws at line 235 (this is new). Previously, release_port(_port) ran unconditionally right before _app.run(), so this path always released the port. Now it doesn't — the reservation socket is leaked for the remaining lifetime of the test process.
• The pidfile-wait failure → throws at lines 270-276 (this is new). Same issue: this used to run after the (now-removed) early release, so it was already safe; now it runs before the moved release call and leaks.

Since server_startup_st::_start_server() (libtest/server_container.cc:298-365) catches libtest::disconnected and continues running the rest of the test suite, these aren't process-fatal — they're a genuine resource/port leak that accumulates for the rest of the binary's run whenever a server fails to launch (fork/exec failure, missing pidfile). This is precisely the kind of edge case the stress-test plan (which exercises the happy path plus one deliberate bind-conflict scenario) wouldn't catch.

Suggested fix: guarantee the release via RAII instead of a single call at the end of the happy path, e.g. a small scope-guard constructed right after args() succeeds:

struct PortReleaser {
  in_port_t port;
  ~PortReleaser() { libtest::release_port(port); }
} release_guard{_port};

placed before _app.run(), so every exit path (success, ping timeout, or exception) releases exactly once.

Secondary/minor issue: auxiliary ports not tied to Server::_port

Several call sites get a port via get_free_port() for something other than the Server object's own _port, e.g.:

• tests/httpd_test.cc:149 — http_port is passed as --http-port= to a gearmand instance whose Server::_port is actually default_port(). Server::start() only ever releases its own _port, so http_port's reservation is now held open for the rest of the process (previously it was released immediately in get_free_port()).
• tests/gearmand.cc:190,549 — --check-args runs get a port via get_free_port() that's never passed through a Server at all.

Functionally this doesn't reintroduce the race (over-holding a reservation is safe, just wasteful), but it's a straightforward fd/port leak for the remainder of the test binary in these paths. Not blocking, but worth a follow-up or a one-line note in the PR description since it's a direct consequence of the design (only Server::start()'s own _port gets released).

Test coverage

• The manual test plan (build, t/unittest, t/cycle's bind-conflict test, and concurrent stress runs) is solid for validating the intended race fix and doesn't regress the happy path.
• It does not cover the _app.run()-failure or pidfile-wait-failure throw paths in Server::start(), which is exactly where the regression above lives — understandably hard to trigger deliberately (needs fork/exec failure), but worth at least a unit test that forces Application::run() to fail and asserts the port becomes available again afterward.

Security

No security-relevant surface here — this is test-infrastructure-only code (libtest/), not shipped in the gearmand/libgearman production binaries.

Summary

The primary fix is correct and well-reasoned, but moving the single release_port() call to the very end of Server::start() trades the old bug (released too early) for a new, narrower one (not released at all on certain throw paths). I'd recommend wrapping the release in RAII before merging so it's unconditionally guaranteed, and optionally noting the auxiliary-port leak (httpd_test.cc et al.) as a known, low-severity follow-up.

🤖 Generated with Claude Code

@esabol, unfortunately, I wan't be able to get to this until Sunday.

@esabol, unfortunately, I wan't be able to get to this until Sunday.

That's OK. I'd like to include this in 2.0.0. I think one more week won't make a big difference. Thank you for your contributions!

This all sounds very promising. It would explain the intermittent test failures we've seen for over a decade, and I hope it will eliminate the need to retry the test stage in the CI.